Skip to content

fix permissions and add specs for domain and dp#25782

Closed
siddhant1 wants to merge 2 commits intomainfrom
sid/fix-permissions
Closed

fix permissions and add specs for domain and dp#25782
siddhant1 wants to merge 2 commits intomainfrom
sid/fix-permissions

Conversation

@siddhant1
Copy link
Member

@siddhant1 siddhant1 commented Feb 10, 2026

Describe your changes:

Fixes

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Route-level authorization:
    • Wrapped /domain/* and /dataProduct routes with AdminProtectedRoute checking ViewBasic or ViewAll permissions
  • Component-level permission checks:
    • Added permission validation in DomainDetailPage.component.tsx and DataProductsPage.component.tsx to handle both frontend checks and backend 403 responses
    • Reset isForbidden state on each fetch to prevent stale permission errors when navigating between entities
  • Permission error handling:
    • Display ErrorPlaceHolder with permission message when users lack domain or data product view permissions
    • Separated frontend permission checks from backend 403 response handling for clearer logic flow
  • New E2E test:
    • DomainViewPermissions.spec.ts validates permission denial for domain and data product listing and detail pages

This will update automatically on new commits.


@github-actions
Copy link
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@gitar-bot
Copy link

gitar-bot bot commented Feb 10, 2026

🔍 CI failure analysis for 8e8825b: Two Playwright shards failed with flaky tests unrelated to PR changes. Shard 2/6: persistent KPI Widget rendering timeout. Shard 3/6: 10 tests failed initially, 9 recovered on retry including the one test involving domain/data products.

Issue

Two Playwright E2E job shards failed with multiple test failures:

  • Shard 2/6 (job 63071943327): 1 test failed - KPI Widget chart rendering
  • Shard 3/6 (job 63071943337): 10 tests failed initially, but 9 passed on retry

Root Cause

All failures are not related to the PR changes. This PR only modifies:

  • Domain and Data Product permission checks (DomainDetailPage.component.tsx, DataProductsPage.component.tsx)
  • Route-level authorization (AuthenticatedAppRouter.tsx)
  • Permission E2E tests (DomainViewPermissions.spec.ts)
  • Unit test mocks (DataProductsPage.component.test.tsx)

The failing tests exercise completely different functionality: bulk editing, glossary operations, impact analysis, service permissions, and entity restoration.

Details

Shard 2/6 - KPI Widget Test (Persistent Failure)

Test: KPI Widget @sample-data in CustomizeWidgets.spec.ts

Error:

Error: expect(locator).toBeVisible() failed
Locator: getByTestId('KnowledgePanel.KPI').locator('.recharts-area')
Timeout: 5000ms
Error: element(s) not found

Pattern: Failed on both initial run and retry #1. This test exercises home page KPI widget rendering, which is unrelated to domain/data product permission enforcement.

Shard 3/6 - Multiple Test Failures (9 Recovered on Retry)

Failed Tests (all recovered on retry except one):

  1. Bulk Edit Entity - Glossary Term (Nested)
  2. Bulk Edit Entity - Database Schema
  3. Glossary Asset Operations - remove glossary term tag
  4. Glossary Hierarchy - move term with children
  5. Impact Analysis - Downstream connections
  6. Impact Analysis - upstream/downstream counts for column level
  7. Impact Analysis - entity popover card on hover
  8. Glossary Permissions - Team-based permissions
  9. SearchIndex Service Permissions - allow common operations
  10. Table - Restore with Inherited domain and data products ✓ (passed on retry)

Error Patterns:

  • Element not found timeouts
  • Page/browser closed prematurely
  • Strict mode violations (multiple matching elements)

Notably, test #10 which involves "domain and data products" passed successfully on retry, demonstrating that the permission changes in this PR do not cause functional issues with domain/data product operations.

Analysis

These are flaky test failures caused by timing issues, race conditions, or test environment instability:

  • 9 out of 10 tests in shard 3/6 passed on automatic retry
  • The one test explicitly related to domain/data products (RestoreEntityInheritedFields for Table) passed on retry
  • KPI Widget test shows consistent timing issues with chart rendering
  • No failures occur in the new DomainViewPermissions.spec.ts test added by this PR

The failures are environmental/timing-related and not caused by the permission enforcement changes in this PR.

Code Review ✅ Approved 1 resolved / 1 findings

Clean implementation of domain and data product view permission checks at both route and component levels. The previous finding about isForbidden never being reset has been properly addressed with setIsForbidden(false) at the start of each fetch function.

✅ 1 resolved
Bug: isForbidden is never reset, causing sticky permission errors

📄 openmetadata-ui/src/main/resources/ui/src/components/DataProducts/DataProductsPage/DataProductsPage.component.tsx:66 📄 openmetadata-ui/src/main/resources/ui/src/components/Domain/DomainDetailPage/DomainDetailPage.component.tsx:56
In both DataProductsPage and DomainDetailPage, the isForbidden state is initialized to false and set to true when a 403 response is received, but it is never reset back to false.

If the React component instance is reused when the route parameter changes (e.g., navigating from /dataProduct/forbidden-one to /dataProduct/allowed-one), the useEffect that depends on dataProductFqn will fire, but since isForbidden is already true, the component renders the error placeholder before the fetch completes. Even if the new entity is accessible, the user will still see the permission error.

Fix: Reset isForbidden to false at the beginning of each fetch function:

// In DataProductsPage - fetchDataProductByFqn
const fetchDataProductByFqn = async (fqn: string) => {
  setIsMainContentLoading(true);
  setIsForbidden(false); // Reset on each fetch
  try {
    // ...
  }
};

// In DomainDetailPage - fetchDomainByName  
const fetchDomainByName = async (domainFqn: string) => {
  setIsMainContentLoading(true);
  setIsForbidden(false); // Reset on each fetch
  try {
    // ...
  }
};

This also requires moving the isForbidden check to after the isMainContentLoading check to avoid a brief flash of the permission error while the new entity is loading.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@github-actions
Copy link
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.75% (56061/85258) 45.14% (29340/65004) 47.91% (8851/18474)

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant